fix(web): pin image annotations to the picture, not the letterboxed box - #236
Conversation
|
Ran a deep multi-agent review against the actual worktree (with tests/tsc runnable, not just the diff). Found four confirmed issues worth addressing before merge: Correctness gap: Reuse: Reuse: the jsdom geometry-stub helper is copy-pasted verbatim into three new test files (wipe-viewer.test.tsx, compare-overlay.test.tsx, image-viewer-annotation-frame.test.tsx). A shared test-utils helper would avoid three-way drift. Test coverage: side-by-side compare pane A has no geometry-pinning regression test, only pane B does. Confirmed by stripping ImageFrameConstraint from pane A only and running the suite, the full 282-test suite still passes. Will push fixes for these. |
Drawings on images were authored and replayed against the full container box rather than the picture inside it. `use-drawing` sized the Fabric canvas from the parent container, stored that size as `_canvasWidth`/`_canvasHeight`, and `AnnotationOverlay` rescaled by a plain per-axis ratio on display. That rescale is only correct when the display container has the same aspect ratio as the authoring one, so any change of shape (sidebar collapsed vs expanded, a resized window, a compare pane, a share link) both stretched the drawing and moved the picture underneath it. Measured in a browser: a 3:1 image annotated at 25% down the picture in a 900x500 viewer, reopened at 400x700, put the mark at -29% — 54% of the image height adrift and off the picture entirely. Through the new constraint it lands at 25% exactly. Video never had this bug; `VideoFrameConstraint` already fits the overlay to the rendered video box. This is the image counterpart, but it cannot be a copy. `<video>` is `w-full h-full object-contain`, so it fills its container and the contain fit can be derived from the container. Images are `max-w-full max-h-full`, and `max-*` only ever shrinks, so an image smaller than its container renders at natural size rather than scaling up. Deriving the box from the container upscales it in that case — verified: a 100x200 image in an 800x400 box really occupies 100x200 at (350,100), where container-derived math claims 200x400 at (300,0). `renderedImageBox` therefore runs the contain fit inside the element's own box and offsets by where that element sits, which is correct for both patterns. It reads offset* rather than a client rect because this renders inside react-zoom-pan-pinch's transform, where a client rect would fold in the zoom matrix. Applied to all four surfaces that show image annotations: the single viewer (which also covers the folder share viewer, since that routes through ImageViewer), both side-by-side compare panes, and the wipe view. Wipe is not optional — once authoring moves to image space, a viewer left in container space becomes newly wrong rather than merely inconsistent. `AnnotationOverlay` and `AnnotationCanvas` are unchanged: both size themselves from their parent, so mounting them inside the constraint is the whole mechanism. Note for existing data: image annotations saved before this change were only ever correct in the exact container they were drawn in, and are now interpreted in image space, so they will shift. Closes #185
Review follow-ups on #185. ResizeObserver watched the <img> alone. Under `max-w-full max-h-full` an image smaller than its container renders at natural size, so a container resize only RECENTRES it: offsetLeft/offsetTop move while the element's own box is untouched, and ResizeObserver does not fire on a position-only change. The overlay stayed pinned where the picture used to be, which is the same drift #185 set out to fix, for the case where the picture never needs shrinking. It now observes the container as well. Also folds the contain fit into one helper. `renderedImageBox` and `VideoFrameConstraint` computed the same centred, aspect-preserving box two different ways (cross-multiplication vs division); both now call `containBox`, leaving only the reference box different, which is the part that genuinely must differ. Checked equivalent for integer video dimensions: no branch flips, differences bounded at ~1e-13px. The video overlay also fills its container instead of collapsing to 0x0 when measured before layout. Tests 282 -> 293: - A ResizeObserver stub that records its target, so a resize can be delivered to the container and not the picture. The existing resize test fired every callback regardless of target and passed either way. - VideoFrameConstraint had no direct coverage (the compare and wipe suites stub ResizeObserver precisely to route around its math). Pinned before moving it, and mutation checked. - A pane A geometry test for side-by-side compare. Only pane B had one, so stripping pane A's constraint left the suite green; this fails for both a removed constraint and a swapped ref. - The jsdom geometry stub was copy-pasted into three suites, now one helper in test/geometry.ts.
…s element Blind review of this PR found three things. Every annotation saved before this change is silently reinterpreted. The overlay sizes its Fabric canvas from its parent and rescales stored objects by w/_canvasWidth, and this PR moves that parent from the letterboxed container to the picture box, so stored CONTAINER dimensions were being divided into PICTURE dimensions. A mark drawn 25% down a 3:1 image in a 900x500 viewer landed 35% down with a 40% vertical squash — in the very viewport it was authored in, where the old code placed it exactly right. Saved data is now replayed against the container it was authored for and then shifted into the picture box, which reproduces its original position, and new data records its coordinate space so nothing has to be inferred. Video is unaffected: VideoFrameConstraint predates this, so unmarked video annotations are already picture-space and are left alone. The overlay measured its box exactly once. That was safe when its parent was permanently `absolute inset-0`, but the constraint now starts as the whole pane (an undecoded <img> reports no size) and shrinks to the picture on load, so an annotation opened from a ?commentId= deep link was scaled against the pane. It now re-measures on resize, as AnnotationCanvas already did. The claim that <video> always fills its container is false: the compare panes render `max-h-full max-w-full`, the same shrink-only case the <img> work exists for, so a video smaller than its pane had its overlay upscaled and every annotation misplaced. Both constraints now measure the element's own box, which is correct for filling and shrink-only alike, so the helper is genuinely shared rather than two rules with a caveat.
c3c6689 to
cb38b64
Compare
The first pass at legacy compatibility replayed old annotations against the container they were authored in, reproducing their original placement exactly. That was faithful but not useful: those coordinates were always container-relative, so the mark still slid around as the viewer's aspect ratio changed. Manual testing showed exactly that, a mark drifting off the detail it was drawn on. The stored dimensions plus the image's intrinsic size are enough to do better. The picture box inside the authoring container is just the contain fit of the natural size into the stored `_canvasWidth`/`_canvasHeight`, so an old mark can be re-expressed relative to the picture and replayed at any size. Two marks on a 1200x800 image saved in a 520x870 pane sit 32.0% and 30.8% down the picture; they now render at that same point whether the pane is 1400x900, 900x500 or 600x1000, where before each pane put them somewhere different. This does move existing annotations relative to where they rendered before, which the CHANGELOG now says plainly. The new position is the one they were drawn at. The context carries the intrinsic size rather than the container box, since that is all the reconstruction needs, and it is null on surfaces that always authored in media space so video data is left alone.
Closes #185.
Image annotations were authored and replayed against the full letterboxed container box rather than the picture inside it, so the same drawing pointed at a different part of the image depending on where it was viewed: sidebar collapsed vs expanded, a resized window, a compare pane, a share link.
The mechanism
use-drawingsized the Fabric canvas from the parent container (use-drawing.ts:65-67), stored that size as_canvasWidth/_canvasHeight(:325-326), andAnnotationOverlayreplayed it with a plain per-axis rescale (annotation-overlay.tsx:65-79). That rescale is only correct when the display container has the same aspect ratio as the authoring one. When it does not, the drawing is stretched non-uniformly and the picture underneath has moved, because the letterbox insets differ.Measured in a browser, a 3:1 image annotated 25% down the picture in a 900x500 viewer and reopened at 400x700:
Why this is not a copy of
VideoFrameConstraintVideo never had this bug, so the obvious fix is to mirror
VideoFrameConstraint. That would have been subtly wrong, because the two elements use different CSS:<video>fills its container and letterboxes internally, so the contain fit can be derived from the container.max-*only ever shrinks a replaced element, so an image smaller than its container renders at natural size and is not scaled up. Verified in Chrome against the exact shipped CSS:max-*max-*w-full h-fullrenderedImageBoxtherefore runs the contain fit inside the element's own box and offsets by where that element sits, which is correct for all three. It readsoffset*rather than a client rect because this renders inside react-zoom-pan-pinch'sTransformComponent, where a client rect would fold the zoom matrix in.Aspect ratios are compared by cross-multiplying rather than dividing, so an exact fit (a 16:9 image in a 16:9 box) stays exact instead of drifting a fraction of a pixel through an intermediate ratio.
Surfaces covered
ImageViewerWipe is not optional scope even though the issue does not mention it. Once authoring moves into image space, a viewer still rendering in container space goes from consistent-but-wrong to newly wrong. Leaving it would have been a regression introduced by this PR.
AnnotationOverlayandAnnotationCanvasare unchanged. Both size themselves from their parent, so mounting them inside the constraint is the whole mechanism.Tests
263 to 282 (+19, 3 new files), full suite green.
Red-green was verified per surface, and again at the end in one pass: reverting only the three wiring files fails exactly the four regression tests, one per surface, while the helper and component unit tests stay green.
The
renderedImageBoxunit tests pin the three browser-measured shapes above, so a future change back to container-derived math fails immediately rather than silently.One consequence to be aware of
Image annotations saved before this change will shift. They were only ever correct in the exact container they were drawn in, and are now interpreted in image space. There is no version marker in the stored Fabric JSON to tell old data from new, so no migration is attempted here. If preserving them matters, the cheap path is to start stamping a frame marker in
getJSON()now so a migration becomes possible later. Happy to add that as a follow-up if you want it.